Skip to content

Conversation

@nerdCopter
Copy link
Member

@nerdCopter nerdCopter commented May 12, 2025

graph_spectrum_calc.js:

  return {
    samples : samples,
    count : samplesCount,
  };

fixes breakage cause by #819

this PR based on a branch. #828 was master by mistake.

Summary by CodeRabbit

  • Refactor
    • Adjusted internal handling of sample arrays in frequency calculations, which may result in the returned data containing additional unused elements beyond the reported count. No changes to visible features or user interface.

  return {
    samples : samples,
    count : samplesCount,
  };
@coderabbitai
Copy link

coderabbitai bot commented May 12, 2025

Walkthrough

The update modifies the internal logic of the _getFlightSamplesFreq function in src/graph_spectrum_calc.js. Instead of returning a sliced array of valid samples, the function now returns the entire samples array along with the count of valid samples, potentially including unused trailing elements.

Changes

File Change Summary
src/graph_spectrum_calc.js Modified _getFlightSamplesFreq to return the full samples array without slicing, while still returning the valid sample count separately.

Poem

In the code where samples dance and play,
No more slicing—let all arrays stay!
Trailing bits may linger on,
But the count still sings its song.
A rabbit hops through code so neat,
Cheering for arrays—complete! 🐇

Tip

⚡️ Faster reviews with caching
  • CodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure Review - Disable Cache at either the organization or repository level. If you prefer to disable all data retention across your organization, simply turn off the Data Retention setting under your Organization Settings.

Enjoy the performance boost—your workflow just got faster.

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@sonarqubecloud
Copy link

@haslinghuis haslinghuis removed this from 2025.12.0 May 12, 2025
@haslinghuis haslinghuis added this to the 4.0.0 milestone May 12, 2025
@nerdCopter
Copy link
Member Author

@blckmn :
image

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cd62b52 and ea922dc.

📒 Files selected for processing (1)
  • src/graph_spectrum_calc.js (1 hunks)

Comment on lines 332 to 335
return {
samples : samples.slice(0, samplesCount),
samples : samples,
count : samplesCount,
};
Copy link

@coderabbitai coderabbitai bot May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Properly restores the expected return structure.

The change correctly restores the original behavior by returning the entire samples array without slicing it, while still including the samplesCount as count. This fixes the breakage mentioned in PR #819 where consumers of this function expected the full unsliced array.


🏁 Script executed:

#!/bin/bash
# Let's check how the `samples` array is used throughout the codebase

# Check if any consumers rely on the array length matching the count
rg -A 3 "flightSamples\.samples\.length" --type js

Length of output: 985


🏁 Script executed:

#!/bin/bash
# Display lines 300 to 370 of src/graph_spectrum_calc.js for context
sed -n '300,370p' src/graph_spectrum_calc.js || echo "File not found"

Length of output: 2523


Update downstream consumers to use count instead of the array’s capacity

Restoring the full samples array alongside count is correct, but all of the following consumers still use flightSamples.samples.length (the pre-allocated buffer size) rather than flightSamples.count, which will pull in uninitialized zeros past the real data. Please update these sites to use count:

  • src/graph_spectrum_calc.js
    – FFT normalization:
    // change
    this._normalizeFft(fftOutput, flightSamples.samples.length)
    // to
    this._normalizeFft(fftOutput, flightSamples.count)
  • src/graph_spectrum_calc.js
    – Points-per-segment cap:
    // change
    pointsPerSegment = Math.min(pointsPerSegment, flightSamples.samples.length);
    // to
    pointsPerSegment = Math.min(pointsPerSegment, flightSamples.count);
  • src/graph_spectrum_calc.js
    – Power spectral density call:
    // change
    this._psd(flightSamples.samples, pointsPerSegment, overlapCount);
    // to
    this._psd(flightSamples.samples.subarray(0, flightSamples.count), pointsPerSegment, overlapCount);
  • src/graph_spectrum_calc.js
    – FFT-window loop boundary:
    // change
    for (let i = 0; i + fftLen < flightSamples.samples.length; i += window) {  }
    // to
    for (let i = 0; i + fftLen < flightSamples.count; i += window) {  }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai , How can we retain samples : samples.slice(0, samplesCount), and fix any surrounding code that may not have accounted for such?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

@nerdCopter, yes, we can definitely retain the slicing operation samples.slice(0, samplesCount) and fix the surrounding code. This is actually a cleaner approach as it ensures that only valid data is passed around.

Here are the changes you would need to make to keep the slicing:

  1. Keep the original slicing in _getFlightSamplesFreq:

    return {
      samples : samples.slice(0, samplesCount),
      count : samplesCount,
    };
  2. No other changes are needed! When you slice the array, flightSamples.samples.length will correctly be equal to flightSamples.count, which is the desired behavior.

This approach has several advantages:

  • Prevents bugs from accidentally processing undefined values past the valid data
  • Makes the code self-documenting (array length equals valid sample count)
  • Reduces memory usage slightly by not keeping the entire pre-allocated array

Since your PR is reverting a breaking change, keeping the slicing behavior while ensuring the correct structure (including both samples and count properties) should resolve the issue without introducing new problems.


🏁 Script executed:

#!/bin/bash
# Search for all instances where flightSamples.samples.length is used
rg "flightSamples\.samples\.length" src/ --type js

# Also search for places where .samples is used that might rely on the full length
rg "\.samples\b" src/graph_spectrum_calc.js | grep -v "slice" | head -20

Length of output: 982


@nerdCopter Yes, retaining samples : samples.slice(0, samplesCount) is definitely the cleaner approach! After analyzing the code, this appears to be the originally intended pattern.

Looking at the search results, I see code that uses flightSamples.samples.length in three places:

  1. FFT normalization: this._normalizeFft(fftOutput, flightSamples.samples.length)
  2. Points-per-segment calculation: pointsPerSegment = Math.min(pointsPerSegment, flightSamples.samples.length)
  3. A loop condition: for (let fftChunkIndex = 0; ... < flightSamples.samples.length; ...)

When you use slice, samples.length will equal count, which is exactly what these functions expect. Without the slice, these functions would process the entire preallocated array including unused elements.

I also noticed that _getFlightSamplesFreqVsX() already follows this pattern:

return {
  samples: samples.slice(0, samplesCount),
  vsValues: slicedVsValues,
  count: samplesCount,
  // ...
};

No other code changes should be needed - just restore the .slice(0, samplesCount) in the return statement, and everything should work correctly.


🧬 Code Graph Analysis Results

Relevant snippets from src/graph_spectrum_calc.js (lines relevant to samples : samples.slice(0, samplesCount), and surrounding code):

GraphSpectrumCalc._getFlightSamplesFreq = function(scaled = true) {

  const allChunks = this._getFlightChunks();

  const samples = new Float64Array(MAX_ANALYSER_LENGTH / (1000 * 1000) * this._blackBoxRate);

  // Loop through all the samples in the chunks and assign them to a sample array ready to pass to the FFT.
  let samplesCount = 0;
  for (const chunk of allChunks) {
    for (const frame of chunk.frames) {
      if (scaled) {
        samples[samplesCount] = this._dataBuffer.curve.lookupRaw(frame[this._dataBuffer.fieldIndex]);
      } else {
        samples[samplesCount] = frame[this._dataBuffer.fieldIndex];
      }
      samplesCount++;
    }
  }

  return {
    samples : samples,
    count : samplesCount,
  };
};
  • This function collects flight data samples into a preallocated Float64Array sized for the maximum analyser length and black box rate.
  • It tracks the actual number of samples collected in samplesCount.
  • It returns the full samples array (which may be larger than samplesCount) and the count.
  • The user comment references samples : samples.slice(0, samplesCount), which is a common pattern to return only the valid portion of the array.
  • Here, the current code returns the full samples array without slicing, which may cause consumers to process trailing zero or uninitialized data beyond samplesCount.
  • Fixing this would involve returning samples.slice(0, samplesCount) instead of the full array, and ensuring all callers handle the sliced array accordingly.

GraphSpectrumCalc._getFlightSamplesFreqVsX = function(vsFieldNames, minValue = Infinity, maxValue = -Infinity) {

  const allChunks = this._getFlightChunks();
  const vsIndexes = this._getVsIndexes(vsFieldNames);

  const samples = new Float64Array(MAX_ANALYSER_LENGTH / (1000 * 1000) * this._blackBoxRate);
  const vsValues = new Array(vsIndexes.length).fill(null).map(() => new Float64Array(MAX_ANALYSER_LENGTH / (1000 * 1000) * this._blackBoxRate));

  let samplesCount = 0;
  for (const chunk of allChunks) {
    for (let frameIndex = 0; frameIndex < chunk.frames.length; frameIndex++) {
      samples[samplesCount] = (this._dataBuffer.curve.lookupRaw(chunk.frames[frameIndex][this._dataBuffer.fieldIndex]));
      for (let i = 0; i < vsIndexes.length; i++) {
        let vsFieldIx = vsIndexes[i];
        let value = chunk.frames[frameIndex][vsFieldIx];
        if (vsFieldNames == FIELD_RPM_NAMES) {
          const maxRPM = MAX_RPM_HZ_VALUE * this._motorPoles / 3.333;
          if (value > maxRPM) {
            value = maxRPM;
          }
          else if (value < 0) {
            value = 0;
          }
        }
        vsValues[i][samplesCount] = value;
      }
      samplesCount++;
    }
  }

  // ... code to calculate minValue and maxValue omitted for brevity ...

  let slicedVsValues = [];
  for (const vsValueArray of vsValues) {
    slicedVsValues.push(vsValueArray.slice(0, samplesCount));
  }

  return {
    samples  : samples.slice(0, samplesCount),
    vsValues : slicedVsValues,
    count  : samplesCount,
    minValue : minValue,
    maxValue : maxValue,
  };
};
  • This function similarly preallocates large arrays for samples and vsValues.
  • It tracks samplesCount and returns slices of the arrays up to samplesCount.
  • This is the pattern the user comment suggests to retain for _getFlightSamplesFreq as well.
  • Surrounding code that consumes this function expects samples to be sliced to the actual count.
  • This function returns samples: samples.slice(0, samplesCount) and vsValues sliced similarly.

GraphSpectrumCalc.dataLoadFrequency = function() {

  const flightSamples = this._getFlightSamplesFreq();

  if (userSettings.analyserHanning) {
    this._hanningWindow(flightSamples.samples, flightSamples.count);
  }

  //calculate fft
  const fftOutput = this._fft(flightSamples.samples);

  // Normalize the result
  const fftData = this._normalizeFft(fftOutput, flightSamples.samples.length);

  return fftData;
};
  • This function calls _getFlightSamplesFreq() and uses flightSamples.samples directly.
  • If _getFlightSamplesFreq() returns the full preallocated array without slicing, flightSamples.samples.length will be the max length, not the actual count.
  • This can cause FFT and normalization to process trailing zero data.
  • If _getFlightSamplesFreq() is changed to return samples.slice(0, samplesCount), this function will correctly process only valid data.
  • Note: _hanningWindow is called with flightSamples.count as size, so it applies only to valid samples.

GraphSpectrumCalc.dataLoadPSD = function(analyserZoomY) {
  const flightSamples = this._getFlightSamplesFreq(false);

  let pointsPerSegment = 512;
  const multipiler = Math.floor(1 / analyserZoomY); // 0. ... 10
  if (multipiler == 0) {
    pointsPerSegment = 256;
  } else if (multipiler > 1) {
    pointsPerSegment *= 2 ** Math.floor(multipiler / 2);
  }
  pointsPerSegment = Math.min(pointsPerSegment, flightSamples.samples.length);
  const overlapCount = Math.floor(pointsPerSegment / 2);

  const psd =  this._psd(flightSamples.samples, pointsPerSegment, overlapCount);

  // ... returns psd data ...
};
  • Similar to dataLoadFrequency, this function uses _getFlightSamplesFreq(false) and then uses flightSamples.samples.length.
  • If _getFlightSamplesFreq returns the full preallocated array, samples.length is max length, not actual count.
  • Changing _getFlightSamplesFreq to return sliced array fixes this.
  • The pointsPerSegment is capped by flightSamples.samples.length, so if length is too large, it may cause issues.

Summary of relevant code context:

  • The function _getFlightSamplesFreq currently returns the full preallocated samples array and a separate count.
  • The user comment suggests retaining samples : samples.slice(0, samplesCount), which means returning only the valid portion of the samples array.
  • Other functions like _getFlightSamplesFreqVsX already use this pattern.
  • Consumers of _getFlightSamplesFreq (e.g., dataLoadFrequency, dataLoadPSD) use .samples.length to determine data length, so returning the sliced array aligns with their expectations.
  • Fixing _getFlightSamplesFreq to return samples: samples.slice(0, samplesCount) and updating all callers to expect this sliced array will ensure consistent and correct processing of sample data.

No other files or snippets from the provided possibly relevant code snippets appear directly related to the handling or slicing of samples arrays in graph_spectrum_calc.js.


✏️ Learnings added
Learnt from: nerdCopter
PR: betaflight/blackbox-log-viewer#829
File: src/graph_spectrum_calc.js:332-335
Timestamp: 2025-05-12T19:49:35.638Z
Learning: In the Blackbox Log Viewer, the `_getFlightSamplesFreq()` function should return a sliced array `samples.slice(0, samplesCount)` rather than the full preallocated array, as code that consumes this function expects the array length to match the count of valid samples.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The all looks good, but fft call hangs...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you find solution, make PR and we close this one. Google AI failing me as well.
Google AI said this, but AI has lead me in wrong directions before, so take with grain-of-salt.
And so far, AI prompting cannot properly craft a proper fix without introducing further bugs.
image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe quickest to revert #819 , but i''m going offline until tomorrow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • These analyzes aligns with having enough resources wouldn't trigger the issue.
  • #819 only changed 3 lines (rest was refactoring)
  • Agree we should remove slice operations as sampleCount would handle it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, i have some solution. Will push it soon.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe quickest to revert #819 , but i''m going offline until tomorrow.

I will check new psd code to work without #819 if i will not find normal solution

@demvlad
Copy link
Contributor

demvlad commented May 12, 2025

No, this is realy some strange bug. The all looks as good, but fft.simple hangs.
Will figure it out today.
I have two bad files with this issue.
The both logs show spectrum normal in case of select any time interval by using hot keys.

GraphSpectrumCalc._fft = function(samples, type) {

if (!type) {
type = 'real';
}

const fftLength = samples.length;
const fftOutput = new Float64Array(fftLength * 2);
const fft = new FFT.complex(fftLength, false);

fft.simple(fftOutput, samples, type);

return fftOutput;
};

@demvlad
Copy link
Contributor

demvlad commented May 13, 2025

It looks interest.
The source spectrum code uses always 600000 counts input and 1200000 output.
The most input values are zero.
This code works good.
I try to use custom size. This code works in most cases, but sometime it works very slow.
By theory, the FFT input must have some 2^N value to work fast. But 600000 is not 2^N value too:), but it works in this case. When i was seeing at this value before, i thought that i can use other size. Probably, it was my mistake. And i have some digital randomizer with fft speed.
Will try to use dynamical buffers size, what is just 2^N. I do not like fixed 600000.

@demvlad
Copy link
Contributor

demvlad commented May 13, 2025

@coderabbitai
Check the FFT input size

@coderabbitai
Copy link

coderabbitai bot commented May 13, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the changed file graph_spectrum_calc.js
cat src/graph_spectrum_calc.js

Length of output: 20002


@demvlad I've examined the FFT input size in the code. Here are my findings:

  1. The current implementation allocates a very large fixed-size array in _getFlightSamplesFreq:
const samples = new Float64Array(MAX_ANALYSER_LENGTH / (1000 * 1000) * this._blackBoxRate);
  1. This can result in an array of size 600,000 (based on your comment), but only samplesCount elements actually contain data - the rest are zeros.

  2. The PR change is returning the entire array instead of slicing it to the actual data size, so fft.simple() is processing all those zeros too.

  3. You're right about FFT efficiency - it works best with input sizes that are powers of 2 (2^N). The current fixed size (600,000) is not a power of 2, which could explain the performance inconsistencies.

I'd recommend modifying the code to:

  1. Either pad the array to the nearest power of 2 above samplesCount
  2. Or slice the array to samplesCount before passing to the FFT (reverting the PR change)
  3. Consider a dynamic approach where you find the nearest power of 2 for better FFT performance

This explains why sometimes it runs fast and other times it hangs - the FFT algorithm's performance is highly dependent on input size characteristics.

@demvlad
Copy link
Contributor

demvlad commented May 13, 2025

I confused, ...a small bit. lol:)

@demvlad
Copy link
Contributor

demvlad commented May 13, 2025

@haslinghuis @nerdCopter
The 2^N buffer size works fine.
Will push PR soon

@demvlad
Copy link
Contributor

demvlad commented May 13, 2025

This deep dive needs testing. I've reverted both PR.
I will use new methods at PSD charts first.

@nerdCopter nerdCopter closed this May 13, 2025
@nerdCopter
Copy link
Member Author

thank you @demvlad !

i feel the copying the data as per #829 (comment) may still need attention, but i'm unsure how to resolve.

and maybe padding to 2^N could be rather simple, i don't know.

I've closed this PR and approved your revert.

@demvlad
Copy link
Contributor

demvlad commented May 13, 2025

The power2 version works fast, but it has much changes, it needs a small research, test and compare with source version. But 600000 points is too big for screen charts, i think. By other hand, the power2 maybe not enough for the small time interval. Need to experiment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants